fix(core): keep RLAC cycle-detection state per analyzer invocation - #2619
fix(core): keep RLAC cycle-detection state per analyzer invocation#2619ttw225 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesRLAC cycle detection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SessionContext
participant ModelAnalyzeRule
participant RLACSubqueries
participant Optimizer
SessionContext->>ModelAnalyzeRule: create logical plan
ModelAnalyzeRule->>ModelAnalyzeRule: create invocation cycle stack
ModelAnalyzeRule->>RLACSubqueries: rewrite RLAC subqueries with stack
RLACSubqueries->>ModelAnalyzeRule: analyze nested model plan
ModelAnalyzeRule->>Optimizer: return analyzed plan
Optimizer-->>SessionContext: optimized plan
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
goldmedal
left a comment
There was a problem hiding this comment.
Nice catch, and the diagnosis holds up. I verified it locally rather than taking the description on faith:
- Reverting only
model_anlayze.rsto the base commit while keeping the new test fails 3/3, at iteration 0-1 on 6 of 8 threads — the race window is wide, not marginal. - With the fix: concurrency test passes,
cargo test --libis 148/148. - The premise checks out too:
PySessionContext.exec_ctx(Mode::LocalRuntime) is built once inload_mdland reused, so it shares one rule instance, whiletransform_sql_with_ctxre-derives per call. So the race is real on the exec path and currently masked bycall_lock— which makes this a hard prerequisite for step 3, exactly as described. - Completeness looks right: after this change
ModelStackis the only interior mutability left anywhere underlogical_plan/, and every recursion path threads the caller's stack, so transitiveA -> B -> Adetection is preserved. The two droppedanalyze_table_scanparameters wereArc::clones of the rule's own fields at all four call sites.
Two non-blocking suggestions below — neither affects correctness, so treat them as polish rather than gates.
| /// RLAC references A). Allocated per `analyze` invocation and passed down | ||
| /// the recursive calls: the rule instance is shared by every — possibly | ||
| /// concurrent — query on its session context, so this must not live on `self`. | ||
| type ModelStack = Mutex<HashSet<String>>; |
There was a problem hiding this comment.
Now that the stack is per-invocation and never crosses a thread boundary, the Mutex is permanently uncontended — and, more importantly, the type now says the opposite of what this PR just established. Mutex reads as "shared across threads", which is precisely the property being removed.
RefCell would encode the new invariant in the type system: it is !Sync, so any future attempt to move this back onto ModelAnalyzeRule as a field, or to share it across threads, becomes a compile error rather than a silently reintroduced race. That seems worth having right before step 3 removes call_lock and real concurrent traffic starts arriving here.
Secondary benefit: the scoped block in build_model_plan_node that drops the borrow before recursing is load-bearing. If someone later widens it across the recursive call, parking_lot::Mutex (non-reentrant) deadlocks, whereas RefCell panics with a location. The latter is far easier to diagnose.
I prototyped it to make sure this is not just theory — it is a 4-line change:
type ModelStack = RefCell<HashSet<String>>;
// cycle_stack.lock() -> cycle_stack.borrow_mut()
// self.stack.lock() -> self.stack.borrow_mut()
// use parking_lot::Mutex -> use std::cell::RefCellResult: clippy --all-targets --all-features -- -D warnings clean (exit 0, zero warnings, forced fresh analysis), cargo test --lib 148/148, concurrency regression still passes. No Send/Sync obstacles.
(Unrelated and pre-existing, just noting it while we are here: ModelStack / ModelStackGuard are named "stack" but the underlying type is a HashSet, which has no ordering.)
There was a problem hiding this comment.
Switched to RefCell and verified the guardrail: storing it on
ModelAnalyzeRule fails to compile because the shared analyzer rule must be
Send + Sync.
I updated the documentation and explained why the mutable borrow must end before
recursive analysis and ModelStackGuard::drop. I left the stack naming unchanged
as noted.
| let state = ctx.state(); | ||
| let plan = state.create_logical_plan(SQL).await?; | ||
| let optimized = state.optimize(&plan); | ||
| assert!( |
There was a problem hiding this comment.
Minor diagnostics point: because this is an assert! inside the spawned thread, the failure surfaces at the join as
analyzer stress thread panicked: Any { .. }
since join() yields Err(Box<dyn Any + Send>), which has no useful Debug. The informative message — thread 1 iter 0: valid acyclic RLAC query failed under same-context concurrency: ... — only reaches the output via the default panic hook writing to stderr. Both are visible in a normal local run, so this is cosmetic today, but if stderr is filtered or only the final failure line is surfaced, all that remains is Any { .. }.
The closure already returns Result<()>, so returning an error instead threads the full message through the existing handle.join().expect(...)?:
if let Err(e) = state.optimize(&plan) {
return plan_err!(
"thread {tid} iter {iter}: valid acyclic RLAC query failed \
under same-context concurrency: {e}"
);
}While in here, two optional one-liners:
.stack_size(8 * 1024 * 1024)is not required for this fixture — I removed it and the test still passes — so it reads as defensive. A short comment saying so would save the next reader the experiment I just ran. (It is not a bad instinct:test_composite_key_calculationin this same crate does overflow the default stack in a debug build.)- Worth stating in the module doc that this is a probabilistic guard rather than a proof. It reproduces very strongly here, but on a single-core or heavily loaded runner it could pass despite a reintroduced regression, and a green run should not be read as "no race".
There was a problem hiding this comment.
Switched the assertion to return a contextual plan_err!, so failures retain the
thread, iteration, and underlying planning error through join()?. The negative
control still fails 3/3 with the original analyzer implementation.
I kept the 8 MiB stack to match CI and the existing debug plan-analysis test
convention, documented the rationale and probabilistic nature of the stress
test, and left the sibling boolean-assertion cleanup outside this PR.
Verified: cargo check --all-targets,
RUST_MIN_STACK=8388608 cargo test --lib --tests --bins (148 passed),
cargo clippy --all-targets --all-features -- -D warnings, and
cargo fmt --all -- --check.
|
Thanks for the depth here. Both suggestions are worth taking, and I'll apply the |
Why
ModelAnalyzeRulekept its RLAC cycle-detection stack (building_models) as an instance field, cleared at the top of everyanalyze(). A derivedSessionContextholds one rule instance for its whole lifetime, so concurrentoptimize()calls on the same context share — and corrupt — that stack: an 8-thread stress run of a valid acyclic query reliably fails within ~2 iterations with a spurious "Detected a cycle in row level access control conditions" error, and a lateclear()can equally erase the state that would catch a real cycle.Today the wren-core-py binding masks this with a per-context call lock; removing that lock (#2504, step 3) is only safe once this state is per-invocation.
What
analyze()allocates the cycle stack per invocation and threads it through the model-rewrite path (analyze_model→analyze_table_scan/analyze_subquery_alias_model→build_model_plan_node→analyze_rlac_subqueries→analyze_subquery_plan). RLAC subquery recursion passes the caller's stack, so transitive cycles (A → B → A) are still detected.analyze_scopenever touches cycle state and is unchanged.ModelStackGuardborrows the stack (&ModelStack) instead of sharing ownership; cleanup-on-drop behavior is unchanged.analyze_table_scandrops two parameters that every caller filled with clones of the rule's own fields.Detection logic, error message, and
ModelAnalyzeRule::new's signature are unchanged; single-query behavior is identical.Test Plan
analyzer_concurrencyregression: one shared LocalRuntime derived context, 8 Barrier-started threads × 50 iterations planning an acyclic RLAC chain throughSessionState::optimize(the Analyzer only runs there;transform_sql_with_ctxwould build fresh rule instances per call and cannot observe the race). Fails 3/3 on the unfixed rule, passes 5/5 with the fix, ~0.1s.test_rlac_subquery_cycle_detected,test_rlac_self_reference_is_cycle) still pass — real cycles are still rejected.clippy --all-targets --all-features -D warnings,cargo fmt --check, sqllogictest suite.Part of #2504 (step 2 of the plan in the issue comments).
Summary by CodeRabbit
Bug Fixes
Tests